Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

chore: complete pubsub tests #81

Merged
merged 3 commits into from
May 27, 2021
Merged

Conversation

vasco-santos
Copy link
Member

@vasco-santos vasco-santos commented Jan 21, 2021

Context: libp2p/js-libp2p#857 let's remove peerDeps and get node16 in libp2p CI.

This basically grabs the pubsub subsystem tests from js-libp2p and put them here

This PR restructures the pubsub tests to not use the routers as devDependencies in the libp2p repo, delegating the ownership of the integration testing to the modules. As a result, this PR moves the pubsub core tests from libp2p to the interface. This way, routers will run them as compliance tests with libp2p.

Discussion point/suggestion:

  • Perhaps libp2p interface tests should start receiving an instance of libp2p configured with the module to test, instead of the module instance. This would allow us to create the test using libp2p API instead of the module, which for example to dial other peers mean accessing the libp2p instance within the module context.

Unblocks:

@vasco-santos vasco-santos force-pushed the chore/complete-pubsub-tests branch from 17ba126 to e21a5f0 Compare May 25, 2021 10:30
@vasco-santos vasco-santos marked this pull request as ready for review May 25, 2021 14:37
@vasco-santos
Copy link
Member Author

@achingbrain let me know your thoughts. Specially, if we should keep the same interface testing behaviour of receiving the libp2p module instance for testing, or if we should change all the interface tests to receive the libp2p instance. This can also be a follow up PR to not break the consistency with the other interfaces now

@vasco-santos vasco-santos requested a review from achingbrain May 25, 2021 14:40
@achingbrain
Copy link
Member

Perhaps libp2p interface tests should receive an instance of libp2p instead of the module instance

My gut feeling here is that this is not a good idea as it turns interface tests into integration tests which may hinder our ability to completely exercise the interface during the test, if some aspects of the interface require certain setup or other conditions.

@achingbrain
Copy link
Member

Unrelated but part of me thinks we should split the interface tests from the interfaces themselves, otherwise you end up with aegir being an undeclared dep of code that can be required when running in production.

@vasco-santos
Copy link
Member Author

My gut feeling here is that this is not a good idea as it turns interface tests into integration tests which may hinder our ability to completely exercise the interface during the test, if some aspects of the interface require certain setup or other conditions.

Yeah, I agree let's wait for testground for more complex integration tests.

The PR as it is now, moves the pubsub subsystem tests from libp2p to the interface, which is how libp2p expects the routers to work and to have consistent behaviours. So, while they were technically used as integration tests in libp2p, they really are interface tests.

Unrelated but part of me thinks we should split the interface tests from the interfaces themselves, otherwise you end up with aegir being an undeclared dep of code that can be required when running in production.

I have the same opinion. It is super weird to have the tests in the src folder but this is the result of past decisions. You already opened #58 in the past. I can also promote it to the maintenance weekly candidates of libp2p.

@vasco-santos vasco-santos merged commit 46899b6 into master May 27, 2021
@vasco-santos vasco-santos deleted the chore/complete-pubsub-tests branch May 27, 2021 08:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants